Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Switch fxa_client from using mockiato to using mockall #5890

Closed
wants to merge 21 commits into from
Closed

Switch fxa_client from using mockiato to using mockall #5890

wants to merge 21 commits into from

Conversation

nox1134
Copy link
Contributor

@nox1134 nox1134 commented Oct 27, 2023 •
edited
Loading

ref: #5368
Hi! This is my first time contributing to mozilla so Please let me know of any breaking changes or improvements!
P:S i tried setting up the environment but was having issue while using cargo test

Pull Request checklist

  • Breaking changes: This PR follows our breaking change policy
    • This PR follows the breaking change policy:
      • This PR has no breaking API changes, or
      • There are corresponding PRs for our consumer applications that resolve the breaking changes and have been approved
  • Quality: This PR builds and tests run cleanly
    • Note:
      • For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
      • If this pull request includes a breaking change, consider cutting a new release after merging.
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry in CHANGELOG.md or an explanation of why it does not need one
    • Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
  • Dependencies: This PR follows our dependency management guidelines
    • Any new dependencies are accompanied by a summary of the due dilligence applied in selecting them.

Branch builds: add [firefox-android: branch-name] to the PR title.

@codecov-commenter
Copy link

codecov-commenter commented Oct 27, 2023 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f3cd740) 36.65% compared to head (072f525) 25.80%.

Additional details and impacted files 
@@             Coverage Diff             @@
##             main    #5890       +/-   ##
===========================================
- Coverage   36.65%   25.80%   -10.86%     
===========================================
  Files         348      375       +27     
  Lines       33622    47759    +14137     
===========================================
  Hits        12324    12324               
- Misses      21298    35435    +14137
Files Coverage Δ
components/fxa-client/src/device.rs 0.00% <ø> (ø)
components/fxa-client/src/internal/device.rs 0.00% <ø> (ø)
components/fxa-client/src/internal/http_client.rs 0.00% <ø> (ø)
components/fxa-client/src/internal/mod.rs 0.00% <ø> (ø)
components/fxa-client/src/internal/oauth.rs 0.00% <ø> (ø)
.../fxa-client/src/internal/oauth/attached_clients.rs 0.00% <ø> (ø)
components/fxa-client/src/internal/profile.rs 0.00% <ø> (ø)
components/fxa-client/src/internal/push.rs 0.00% <ø> (ø)

... and 165 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mhammond
Copy link
Member

Thanks for your contribution, and welcome! Testing the full set of crates is tricky to set up, but seeing you are only touching the fxa-client crate you might have a little more luck with just, say cargo test -p fxa-client. You will also need to follow the instructions in docs/building.md, particularly verifying that the verify-desktop-environment.sh script works for you.

If you get stuck on this let us know and we'd be happy to help, but be sure to include the output from your commands.

@nox1134
Copy link
Contributor Author

nox1134 commented Oct 30, 2023 •
edited
Loading

Hi @mhammond ! I followed all the instructions given in docs/building.md but when i trying to run cargo test -p fxa-client , im getting the following error

cargo test -p fxa-client
  Downloaded predicates-tree v1.0.9
  Downloaded mockall_derive v0.11.4
  Downloaded predicates-core v1.0.6
  Downloaded similar v2.3.0
  Downloaded mockall v0.11.4
  Downloaded termtree v0.4.1
  Downloaded colored v2.0.4
  Downloaded mockito v0.31.1
  Downloaded 8 crates (190.0 KB) in 3.01s
   Compiling proc-macro2 v1.0.69
   Compiling unicode-ident v1.0.12
   Compiling serde_json v1.0.108
   Compiling log v0.4.20
   Compiling unicase v2.7.0
   Compiling memchr v2.6.4
   Compiling minimal-lexical v0.2.1
   Compiling fs-err v2.9.0
   Compiling plain v0.2.3
   Compiling siphasher v0.3.11
   Compiling askama_escape v0.10.3
   Compiling glob v0.3.1
   Compiling heck v0.4.1
   Compiling rustix v0.38.21
   Compiling nom v7.1.3
   Compiling unicode-normalization v0.1.22
   Compiling unicode-bidi v0.3.13
   Compiling nss_build_common v0.1.0 (/Users/priyanshi/Desktop/application-services/components/support/rc_crypto/nss/nss_build_common)
   Compiling quote v1.0.33
   Compiling syn v2.0.38
   Compiling nss_sys v0.1.0 (/Users/priyanshi/Desktop/application-services/components/support/rc_crypto/nss/nss_sys)
   Compiling tokio v1.33.0
   Compiling idna v0.4.0
   Compiling mime_guess v2.0.4
   Compiling core-foundation v0.9.3
   Compiling indexmap v1.9.3
   Compiling url v2.4.1
   Compiling httparse v1.8.0
   Compiling http v0.2.9
   Compiling askama_parser v0.1.1
   Compiling weedle2 v4.0.0
   Compiling rand_core v0.6.4
   Compiling tracing v0.1.40
   Compiling security-framework-sys v2.9.1
   Compiling native-tls v0.2.11
error: failed to run custom build command for `nss_sys v0.1.0 (/Users/priyanshi/Desktop/application-services/components/support/rc_crypto/nss/nss_sys)`

Caused by:
  process didn't exit successfully: `/Users/priyanshi/Desktop/application-services/target/debug/build/nss_sys-7c4a1606c342c5b2/build-script-build` (exit status: 101)
  --- stdout
  cargo:rerun-if-env-changed=NSS_DIR

  --- stderr
  thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: NoNssDir', components/support/rc_crypto/nss/nss_sys/build.rs:6:34
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
warning: build failed, waiting for other jobs to finish...

@nox1134
fix lifetime name error
13957c2 
Signed-off-by: Priyanshi Gaur <[email protected]>
@nox1134
minor changes
5db7dc8 
Signed-off-by: Priyanshi Gaur <[email protected]>
@skhamis
Copy link
Contributor

skhamis commented Oct 30, 2023 

error: failed to run custom build command for `nss_sys v0.1.0 (/Users/priyanshi/Desktop/application-services/components/support/rc_crypto/nss/nss_sys)`

Caused by:
  process didn't exit successfully: `/Users/priyanshi/Desktop/application-services/target/debug/build/nss_sys-7c4a1606c342c5b2/build-script-build` (exit status: 101)
  --- stdout
  cargo:rerun-if-env-changed=NSS_DIR

  --- stderr
  thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: NoNssDir', components/support/rc_crypto/nss/nss_sys/build.rs:6:34
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
warning: build failed, waiting for other jobs to finish...

Hi @nox1134!

This usually indicates that NSS failed to build successfully. Does running ./libs/verify-desktop-environment.sh return a success?

nox1134 and others added 11 commits October 31, 2023 01:55
@nox1134
minor changes pt.2
83b1b5f 
Signed-off-by: Priyanshi Gaur <[email protected]>
@nox1134
minor changes pt.3()hopefully final)
dd43a44 
Signed-off-by: Priyanshi Gaur <[email protected]>
@nox1134
minor changes pt.4
a42bc3d 
Signed-off-by: Priyanshi Gaur <[email protected]>
@nox1134
minor changes pt.5
aaaac3f 
Signed-off-by: Priyanshi Gaur <[email protected]>
@nox1134
minor changes pt.6
a2dc6ec 
Signed-off-by: Priyanshi Gaur <[email protected]>
@nox1134
minor changes pt.7
9841e1f 
Signed-off-by: Priyanshi Gaur <[email protected]>
@nox1134
minor changes pt.8
5b5a518 
Signed-off-by: Priyanshi Gaur <[email protected]>
@nox1134
Merge branch 'main' into issue-5368
d93f5b4
@nox1134
minor changes pt.9(fix moving non-Copy variable into a closure)
bba67ee 
Signed-off-by: Priyanshi Gaur <[email protected]>
@nox1134
minor changes pt.10
88809a8 
Signed-off-by: Priyanshi Gaur <[email protected]>
@nox1134
minor changes pt.11
072f525 
Signed-off-by: Priyanshi Gaur <[email protected]>
@nox1134
Copy link
Contributor Author

nox1134 commented Oct 31, 2023 •
edited
Loading 

error[E0507]: cannot move out of `server_ret`, a captured variable in an `FnMut` closure
    --> components/fxa-client/src/internal/oauth.rs:1006:40
     |
993  |         let mut server_ret = HashMap::new();
     |             -------------- captured outer variable
...
1006 |             .returning(|_, _, _, _| Ok(server_ret));
     |                        ------------    ^^^^^^^^^^ move occurs because `server_ret` has type `std::collections::HashMap<std::string::String, internal::http_client::ScopedKeyDataResponse>`, which does not implement the `Copy` trait
     |                        |
     |                        captured by this `FnMut` closure

I tried addressing this issue by following ways:

  1. cloning the server_ret variable into the closure
let cloned_server_ret = server_ret.clone();

Error:

error[E0599]: the method `clone` exists for struct `HashMap<String, ScopedKeyDataResponse>`, but its trait bounds were not satisfied
   --> components/fxa-client/src/internal/oauth.rs:1006:51
    |
1006 |             .returning(|_, _, _, _| Ok(server_ret.clone()));
    |                                                   ^^^^^ method cannot be called on `HashMap<String, ScopedKeyDataResponse>` due to unsatisfied trait bounds
    |
   ::: components/fxa-client/src/internal/http_client.rs:902:1
    |
902  | pub struct ScopedKeyDataResponse {
    | -------------------------------- doesn't satisfy `_: Clone`
    |
   ::: /home/circleci/.rustup/toolchains/1.72.0-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/collections/hash/map.rs:214:1
    |
214  | pub struct HashMap<K, V, S = RandomState> {
    | ----------------------------------------- doesn't satisfy `_: Clone`
    |
    = note: the following trait bounds were not satisfied:
            `internal::http_client::ScopedKeyDataResponse: Clone`
            which is required by `std::collections::HashMap<std::string::String, internal::http_client::ScopedKeyDataResponse>: Clone`

so i annotated the internal::http_client::ScopedKeyDataResponse with #[derive(Clone)]
but still the issue wasnt resolved

error[E0599]: the method `clone` exists for struct `HashMap<String, ScopedKeyDataResponse>`, but its trait bounds were not satisfied
  --> components/fxa-client/src/internal/oauth.rs:1008:51
   |
1008 |             .returning(|_, _, _, _| Ok(server_ret.clone()));
   |                                                   ^^^^^ method cannot be called on `HashMap<String, ScopedKeyDataResponse>` due to unsatisfied trait bounds
   |
  ::: components/fxa-client/src/internal/http_client.rs:902:1
   |
902  | pub struct ScopedKeyDataResponse {
   | -------------------------------- doesn't satisfy `_: Clone`
   |
  ::: /home/circleci/.rustup/toolchains/1.72.0-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/collections/hash/map.rs:214:1
   |
214  | pub struct HashMap<K, V, S = RandomState> {
   | ----------------------------------------- doesn't satisfy `_: Clone`
   |
   = note: the following trait bounds were not satisfied:
           `internal::http_client::ScopedKeyDataResponse: Clone`
           which is required by `std::collections::HashMap<std::string::String, internal::http_client::ScopedKeyDataResponse>: Clone`
help: consider annotating `internal::http_client::ScopedKeyDataResponse` with `#[derive(Clone)]`
  -->  components/fxa-client/src/internal/http_client.rs:902:1
   |
902  + #[derive(Clone)]
903  |             .expect_check_refresh_token_status()
   |
  1. Using the Rc<RefCell<...>> pattern to allow mutable access to non-Copy variables within closures
    but this also gave the same error

@nox1134 nox1134 mentioned this pull request Nov 5, 2023
Merged
5 tasks
@nox1134 nox1134 closed this Nov 5, 2023
@nox1134
Copy link
Contributor Author

nox1134 commented Nov 5, 2023

I'm moving this to a new PR. Please review it
cc: @mhammond @skhamis

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nox1134 @codecov-commenter @mhammond @skhamis